Skip to content

Add production FlatPolygon browser smoke coverage - #92

Merged
xinaesthete merged 1 commit into
mainfrom
codex/diagnose-production-flatpolygonlayer
Jul 28, 2026
Merged

Add production FlatPolygon browser smoke coverage#92
xinaesthete merged 1 commit into
mainfrom
codex/diagnose-production-flatpolygonlayer

Conversation

@xinaesthete

@xinaesthete xinaesthete commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix FlatPolygonLayer model initialization and preserve Deck’s shader assembler hooks.
  • Add a focused unit test for shader assembler propagation.
  • Add a production-consumer Vite/Playwright smoke test covering canonical polygon rendering and WebGL errors.
  • Run the browser smoke test in CI with generated fixture caching and artifact uploads.

Testing

  • FlatPolygonLayer unit coverage added.
  • Production Chromium browser smoke test configured and passed locally, including rendered frames and error checks.

Summary by CodeRabbit

  • Bug Fixes

    • Improved FlatPolygon rendering reliability by ensuring required shader support is available.
    • Prevented unnecessary rendering-model recreation during initial layer updates.
  • Tests

    • Added production browser smoke tests validating polygon rendering, WebGL availability, frame generation, and error handling.
    • Added automated Chromium test coverage for the FlatPolygon consumer, including screenshots and test artifacts.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@xinaesthete, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b5684332-1da1-4134-811a-644ea3c31070

📥 Commits

Reviewing files that changed from the base of the PR and between 2d677c9 and d2f9e0b.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • .github/workflows/test.yml
  • .gitignore
  • package.json
  • packages/layers/src/FlatPolygonLayer.ts
  • packages/layers/tests/flatPolygonLayer.spec.ts
  • tests/production/browser/index.html
  • tests/production/browser/playwright.config.ts
  • tests/production/browser/polygon-shapes.spec.ts
  • tests/production/browser/src.tsx
  • tests/production/browser/vite.config.ts
📝 Walkthrough

Walkthrough

FlatPolygon model creation now preserves the layer shader assembler and avoids unnecessary initial recreation. A production consumer smoke harness renders canonical polygon fixtures, validates runtime behavior with Playwright, and runs in a dedicated CI job.

Changes

FlatPolygon validation

Layer / File(s) Summary
FlatPolygon model wiring and unit coverage
packages/layers/src/FlatPolygonLayer.ts, packages/layers/tests/flatPolygonLayer.spec.ts
Model recreation is conditional during initial updates, and custom models receive the layer shader assembler. A unit test verifies the constructor option.
Production consumer smoke harness
tests/production/flat-polygon/*
A Vite React consumer loads canonical shape metadata, renders a flat-polygons layer, and Playwright checks WebGL2, canvas output, render frames, network responses, and runtime errors.
Browser test command and CI execution
package.json, .github/workflows/test.yml, test-fixtures, test-results/.last-run.json
A browser test script and CI job prepare fixtures and Chromium, run the smoke test, and upload test results even when tests fail.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant Playwright
  participant Vite
  participant Consumer
  participant Fixtures
  participant Deck
  CI->>Vite: Build and preview consumer
  Playwright->>Vite: Open flat-polygon app
  Consumer->>Fixtures: Fetch shapes metadata
  Consumer->>Deck: Create and render flat-polygons layer
  Deck-->>Consumer: Report frames or errors
  Consumer-->>Playwright: Expose readiness and runtime state
  Playwright-->>CI: Complete smoke-test assertions
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding production FlatPolygon browser smoke coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/diagnose-production-flatpolygonlayer

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
tests/production/flat-polygon/vite.config.ts (2)

12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded node_modules nesting path is fragile to pnpm hoisting changes.

packages/layers/node_modules/@deck.gl/core/dist/index.js assumes a specific pnpm dependency-resolution layout. If hoisting behavior or the pinned @deck.gl/core version changes, this path silently breaks the build with an opaque resolution error.

♻️ Suggested resolution via require.resolve
-const deckCoreRoot = path.join(
-  workspaceRoot,
-  'packages/layers/node_modules/@deck.gl/core/dist/index.js'
-);
+const deckCoreRoot = require.resolve('`@deck.gl/core`', { paths: [path.join(workspaceRoot, 'packages/layers')] });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/production/flat-polygon/vite.config.ts` around lines 12 - 15, Replace
the hardcoded `@deck.gl/core` node_modules path used by deckCoreRoot with
module-resolution logic based on require.resolve, so the configuration locates
the installed package entry point regardless of pnpm hoisting or dependency
layout.

51-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Alias replacements point at single files, not package roots — subpath imports would resolve incorrectly.

Vite/rollup-plugin-alias matches ids that start with find + '/' too, but these replacements are exact dist/index.js file paths. A future @spatialdata/core/<subpath> import would resolve to <...>/index.js/<subpath>, which doesn't exist. Not currently triggered by any import in this diff, but worth a defensive note for future maintainers of this consumer harness.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/production/flat-polygon/vite.config.ts` around lines 51 - 75, Update
the alias replacements for the package-root aliases in the Vite configuration so
subpath imports resolve beneath each package’s dist root rather than beneath a
file path. Preserve the dedicated worker alias and existing external dependency
aliases, and ensure the affected `@spatialdata` and zarrextra package aliases
still resolve their main entry points.
tests/production/flat-polygon/smoke.spec.ts (1)

20-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Ordering-dependent 404/console-error correlation risks flaky failures.

Gating allowedMetadataConsoleErrors against expectedMetadata404Count at event-arrival time assumes the response event for a given 404 always arrives before its corresponding console error. That ordering isn't guaranteed across CDP event delivery, so a console error legitimately caused by an expected 404 could still fail the test if it's processed first.

♻️ Suggested fix: compare aggregate counts after collection instead of gating live
   page.on('console', (message) => {
     if (message.type() !== 'error') return;
-    if (
-      expectedMetadataConsoleError.test(message.text()) &&
-      allowedMetadataConsoleErrors < expectedMetadata404Count
-    ) {
-      allowedMetadataConsoleErrors += 1;
-      return;
-    }
-    consoleErrors.push(message.text());
+    if (expectedMetadataConsoleError.test(message.text())) {
+      allowedMetadataConsoleErrors += 1;
+      return;
+    }
+    consoleErrors.push(message.text());
   });

And after the page interactions complete:

+  expect(allowedMetadataConsoleErrors).toBeLessThanOrEqual(expectedMetadata404Count);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/production/flat-polygon/smoke.spec.ts` around lines 20 - 38, Update the
response and console event handlers in the smoke test to collect expected 404
and matching console-error counts independently, without gating
allowedMetadataConsoleErrors against expectedMetadata404Count during event
arrival. After page interactions complete, compare aggregate counts and exclude
only the number of expected metadata console errors that can be matched to
expected 404 responses, while preserving unexpected 404 URL and remaining
console-error reporting.
test-results/.last-run.json (1)

1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove committed Playwright artifact; deferred to consolidated comment.
See the consolidated comment covering this file and test-fixtures.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-results/.last-run.json` around lines 1 - 4, Remove the committed
Playwright artifact represented by .last-run.json from version control, ensuring
generated test-result files are not included in the change.
packages/layers/src/FlatPolygonLayer.ts (1)

107-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Add coverage for the model recreation branch or track the initial creation more robustly.

This code prevents destroying the model on the first extensionsChanged update from Deck, but packages/layers/tests/flatPolygonLayer.spec.ts only tests _getModel() passes Deck’s shaderAssembler; it has no cases for the first post-initializeState update or a later real extensions change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layers/src/FlatPolygonLayer.ts` around lines 107 - 114, Add tests in
flatPolygonLayer.spec.ts covering updateState after initializeState with
extensionsChanged and no oldProps.extensions, asserting the existing model is
retained, plus a later genuine extensions change with oldProps.extensions
defined, asserting the model is destroyed and recreated. Reuse the existing
_getModel/shaderAssembler test setup and target the updateState model-recreation
branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/test.yml:
- Line 166: Update the actions/checkout step in this job to set
persist-credentials to false, matching the existing react-lint and biome-lint
checkout steps while preserving the current checkout behavior.
- Around line 185-194: Update the “Cache generated test fixtures” step in the
fixture-generation job to use a distinct cache key and path from the full
fixture cache used by the test job. Ensure the corresponding “Generate test
fixtures (Python spatialdata 0.7.2)” step uses that isolated partial-fixture
location and cache-hit output, preventing the 0.7.2-only cache from being
restored as the complete fixture set.

In `@test-fixtures`:
- Line 1: Remove the tracked test-fixtures symlink containing the local absolute
path and remove the generated Playwright state file at
test-results/.last-run.json. Update .gitignore to ignore both test-fixtures/ and
test-results/, ensuring generated fixture and test-result directories can be
recreated by tests and CI without being tracked.

---

Nitpick comments:
In `@packages/layers/src/FlatPolygonLayer.ts`:
- Around line 107-114: Add tests in flatPolygonLayer.spec.ts covering
updateState after initializeState with extensionsChanged and no
oldProps.extensions, asserting the existing model is retained, plus a later
genuine extensions change with oldProps.extensions defined, asserting the model
is destroyed and recreated. Reuse the existing _getModel/shaderAssembler test
setup and target the updateState model-recreation branch.

In `@test-results/.last-run.json`:
- Around line 1-4: Remove the committed Playwright artifact represented by
.last-run.json from version control, ensuring generated test-result files are
not included in the change.

In `@tests/production/flat-polygon/smoke.spec.ts`:
- Around line 20-38: Update the response and console event handlers in the smoke
test to collect expected 404 and matching console-error counts independently,
without gating allowedMetadataConsoleErrors against expectedMetadata404Count
during event arrival. After page interactions complete, compare aggregate counts
and exclude only the number of expected metadata console errors that can be
matched to expected 404 responses, while preserving unexpected 404 URL and
remaining console-error reporting.

In `@tests/production/flat-polygon/vite.config.ts`:
- Around line 12-15: Replace the hardcoded `@deck.gl/core` node_modules path used
by deckCoreRoot with module-resolution logic based on require.resolve, so the
configuration locates the installed package entry point regardless of pnpm
hoisting or dependency layout.
- Around line 51-75: Update the alias replacements for the package-root aliases
in the Vite configuration so subpath imports resolve beneath each package’s dist
root rather than beneath a file path. Preserve the dedicated worker alias and
existing external dependency aliases, and ensure the affected `@spatialdata` and
zarrextra package aliases still resolve their main entry points.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e92f197-e3e2-416b-a533-d1d34eac7620

📥 Commits

Reviewing files that changed from the base of the PR and between 671dd60 and 2d677c9.

⛔ Files ignored due to path filters (2)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
  • test-results/smoke-the-built-consumer-r-e303a-shapes-without-WebGL-errors/flat-polygon.png is excluded by !**/*.png
📒 Files selected for processing (11)
  • .github/workflows/test.yml
  • package.json
  • packages/layers/src/FlatPolygonLayer.ts
  • packages/layers/tests/flatPolygonLayer.spec.ts
  • test-fixtures
  • test-results/.last-run.json
  • tests/production/flat-polygon/index.html
  • tests/production/flat-polygon/playwright.config.ts
  • tests/production/flat-polygon/smoke.spec.ts
  • tests/production/flat-polygon/src.tsx
  • tests/production/flat-polygon/vite.config.ts

Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml Outdated
Comment thread test-fixtures Outdated
@xinaesthete
xinaesthete force-pushed the codex/diagnose-production-flatpolygonlayer branch from 2d677c9 to d2f9e0b Compare July 28, 2026 05:55
@xinaesthete
xinaesthete merged commit 117be04 into main Jul 28, 2026
1 check passed
xinaesthete added a commit that referenced this pull request Jul 28, 2026
`pnpm test:fixtures:generate:0.7.2 -- --output-dir X` forwards the `--`
separator through to the Python script verbatim, and argparse treats every
argument after a bare `--` as positional. The parser has no positionals, so
it exits 2:

  generate_fixtures.py: error: unrecognized arguments: -- --output-dir X

This was latent in #92 rather than new. The job's first CI run passed only
because it shared a fixture cache key with the `test` job and got a cache
hit, so the generation step was skipped entirely. Giving it a distinct key
(the correct fix for that collision) forced a cache miss and ran the step
for the first time, exposing the bad invocation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete added a commit that referenced this pull request Jul 28, 2026
…orkflow lint gate (#93)

* Fix invalid test.yml and add an independent workflow lint gate

#92 left a duplicate `with:` key on the `react-lint` checkout step, which
makes test.yml unparseable. GitHub then cannot evaluate the file's `on:`
triggers, so no job runs and the failure surfaces only as a 0-second run
with no jobs and no logs.

Three changes:

- Remove the duplicate key, restoring test.yml to a parseable state.

- Add .github/workflows/workflow-lint.yml, running actionlint over
  .github/workflows. It is a separate file on purpose: a workflow cannot
  validate itself, so the check that would have caught this is precisely
  the one that could not run. An independent file still parses and reports
  a real file:line annotation. Verified against the broken commit:
  `test.yml:27:9: key "with" is duplicated in element of "steps" section`.

- Give every step in test.yml a distinct `name:`. The root cause was four
  byte-identical `- uses: actions/checkout@v5` anchors: a correct, correctly
  anchored review suggestion for the new production-browser job was also
  applied to react-lint, which already had the setting. Unique names remove
  the ambiguity rather than just detecting its consequences.

Also switches `for i in {1..30}` to `for _` to clear the one pre-existing
shellcheck finding, so the new gate starts green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Fix fixture generation in the production-browser job

`pnpm test:fixtures:generate:0.7.2 -- --output-dir X` forwards the `--`
separator through to the Python script verbatim, and argparse treats every
argument after a bare `--` as positional. The parser has no positionals, so
it exits 2:

  generate_fixtures.py: error: unrecognized arguments: -- --output-dir X

This was latent in #92 rather than new. The job's first CI run passed only
because it shared a fixture cache key with the `test` job and got a cache
hit, so the generation step was skipped entirely. Giving it a distinct key
(the correct fix for that collision) forced a cache miss and ran the step
for the first time, exposing the bad invocation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant